Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ExtractArchiveJob: Fixes job not stopping when cancelled by user, also changes state to use "cancelled" instead of "terminated" #2008

Merged
merged 8 commits into from
Jun 18, 2024

Conversation

scireum-mbo
Copy link
Contributor

@scireum-mbo scireum-mbo commented Jun 17, 2024

Description

Before:
Still unpacking:
image
After unpacking:
image

After fixes:
image

Additional Notes

  • This PR fixes or works on following ticket(s): SIRI-972

Checklist

  • Code change has been tested and works locally
  • Code was formatted via IntelliJ and follows SonarLint & best practices
  • Patch Tasks: Is local execution of Patch Tasks necessary? If so, please also mark the PR with the tag.

If it was cancelled we set the state to "cancelled" instead of "terminated"

Fixes: SIRI-972
@scireum-mbo scireum-mbo added the 🐛 Bugfix Contains only a small fix for an existing bug label Jun 17, 2024
@@ -599,7 +599,12 @@ protected boolean markCompleted(String processId,
return modify(processId, process -> process.getState() != ProcessState.TERMINATED, process -> {
if (process.getState() != ProcessState.STANDBY) {
process.setErrorneous(process.isErrorneous() || !TaskContext.get().isActive());
process.setState(ProcessState.TERMINATED);
//Do not alter the job state if the job was previously cancelled by the user
if (process.getState() != ProcessState.CANCELED) {
Copy link
Contributor

@idlira idlira Jun 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on a second thought, it will be a bit more robust if we specifically check for cancelations.
If the job was cancelled, both date and status will be already set.

if (process.getCancelled() == null) {
  process.setState(ProcessState.TERMINATED);
  process.setCompleted(LocalDateTime.now());
}

OR

if (process.getState() != ProcessState.CANCELED) {
  process.setState(ProcessState.TERMINATED);
  process.setCompleted(LocalDateTime.now());
}

process.setState(ProcessState.TERMINATED);
} else {
process.setState(ProcessState.CANCELED);
}
process.setCompleted(LocalDateTime.now());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved in the if above as the date goes along with the status

Copy link
Member

@jakobvogel jakobvogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(subject to the suggestions by @idlira)

Copy link
Member

@sabieber sabieber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: Could we only set the state message "Das Archiv wurde vollständig entpackt" when the job wasn't cancelled, too? In my opinion showing this is a bit misleading, as not all contains were extracted in that case.

@scireum-mbo scireum-mbo merged commit ebcf28e into develop Jun 18, 2024
3 checks passed
@scireum-mbo scireum-mbo deleted the feature/mbo/SIRI-972 branch June 18, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bugfix Contains only a small fix for an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants